Improve T&C (and all) mobile rendering (CFM-501) #447
Conversation
|
This PR includes all commits found in #419. This PR adds only one commit, but it was large enough that I didn't want to commingle them. |
arlen
force-pushed
the
feature-cfm501-mobileRendering
branch
from
6cc996c to
b25dd03
Compare
| personId: $json['personId'], | ||
| actorPersonId: $json['actorPersonId'], | ||
| personId: (int)$json['personId'], | ||
| actorPersonId: (int)$json['actorPersonId'], |
There was a problem hiding this comment.
I realize this is existing code, but what's to stop somebody from sending a random person ID as either the subject or the actor? The original intent of the /record API was for API Users, not AJAX requests, and API Users are already trusted to make calls on behalf of other users.
We probably need to split recordTAndC into two: one for the "regular" API and one for the AJAX API. The second call can set the Actor Person ID based on the currently authenticated user. Person ID would either (1) be the currently authenticated user, or (2) if the user is an Admin, allow Person ID to be asserted if it is any Person the Actor can manage.
There was a problem hiding this comment.
I agree with this concern. Can we break this issue out into a different ticket independent of this PR since this one doesn't change the existing code (other than to ensure the (int) type is properly cast)?
| // The default mode for CO and platform wide T&Cs is "Explicit Consent". | ||
| // There is no enumeration for this in core, so set the value explicitly to 'EC'. | ||
| $this->set('vv_tandc_mode', 'EC'); |
There was a problem hiding this comment.
This is core code referencing a Plugin concept, so we need to fix this.
Presumably the same concept could apply during Review at Login: as a CO Administrator, I might want to set this to Implied Consent to match the behavior of the Enroller Plugin.
Minimally, it seems like TermsAgreer\Lib\Enum\TAndCEnrollmentModeEnum could become a Core Enumeration, perhaps renamed to something like TAndCAgreementModeEnum. The question is whether we also merge it with TAndCLoginModeEnum. While at first glance it seems like we could, the (unknowable) question is what future enhancements might be requested for the Login Review behavior.
We could, for example, imagine a future request for "Require at Login, but not for Administrators", or "Require at Login, but allow the user to Skip twice". So it probably makes sense to keep the two enums as separate concepts, which unfortunately means we probably also need a second CO Setting to control the Explicit / Implied Consent decision.
There was a problem hiding this comment.
Ok - I will move TermsAgreer\Lib\Enum\TAndCEnrollmentModeEnum to Core.
There was a problem hiding this comment.
The TermsAgreer\Lib\Enum\TAndCEnrollmentModeEnum has been moved to Core TAndCAgreementModeEnum.
… to record agreements on login (CFM-501)
arlen
force-pushed
the
feature-cfm501-mobileRendering
branch
from
e07ad12 to
748c0c8
Compare
|
This PR has been rebased against the latest develop. |
3 participants
No description provided.